fix(k3s): fix cross platform image loading #3437
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary by CodeRabbitRelease Notes
WalkthroughAdds platform-aware image load and pull APIs: K3s gains LoadImagesWithPlatform and platform-aware import flow (ctr with optional --platform), DockerProvider gains PullImageWithOpts and a platform option, tests updated for architecture-specific scenarios, and module deps updated for containerd/platforms and OCI image-spec. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant K3s as K3sContainer
participant Saver as ImageSaver
participant Host as HostFS
participant Ctr as k3s-ctr
Note over Caller,K3s: Platform-aware image load request
Caller->>K3s: LoadImagesWithPlatform(images, platform?)
K3s->>Saver: Save image (with or without platform metadata)
Saver->>Host: write tar file
Host-->>K3s: tar path
K3s->>Ctr: exec "ctr images import [--platform os/arch] <tar>"
Ctr-->>K3s: exit code + output
alt success
K3s-->Caller: success
else failure
K3s-->Caller: error (includes ctr output)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (2)modules/k3s/k3s_test.go (2)
modules/k3s/k3s.go (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mdelapenya
changed the title
LaurentGoderre
force-pushed
the
fix-k3s-load-images
branch
4 times, most recently
from
0701b86 to
c4b42ac
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
modules/k3s/k3s.go (3)
184-187: Update comment to reflect plural images.The function accepts variadic
images ...stringbut the comment says "imports a local image" (singular). Consider updating to "imports local images" for accuracy.Apply this diff:
-// LoadImages imports a local image into the cluster using containerd +// LoadImages imports local images into the cluster using containerd func (c *K3sContainer) LoadImages(ctx context.Context, images ...string) error {
189-189: Update comment to reflect plural images.Similar to
LoadImages, this function handles multiple images but the comment says "imports a local image" (singular).Apply this diff:
-// LoadImagesWithPlatform imports a local image into the cluster using containerd for a specific platform +// LoadImagesWithPlatform imports local images into the cluster using containerd for a specific platform func (c *K3sContainer) LoadImagesWithPlatform(ctx context.Context, images []string, platform *v1.Platform) error {
233-236: Consider improving the error message clarity.The error message "importing image %s" with command output might not be immediately clear to users. Consider a more descriptive message like "failed to import images: %s" or "image import command failed: %s".
Apply this diff:
if exit != 0 { b, _ := io.ReadAll(reader) - return fmt.Errorf("importing image %s", string(b)) + return fmt.Errorf("failed to import images: %s", string(b)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/k3s/go.mod(1 hunks)modules/k3s/k3s.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/k3s/k3s.go (3)
provider.go (1)
ProviderDocker(18-18)image.go (1)
SaveImageOption(19-19)docker.go (1)
SaveDockerImageWithPlatforms(1831-1837)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (1.25.x, modules/k3s) / test: modules/k3s/1.25.x
- GitHub Check: test (1.24.x, modules/k3s) / test: modules/k3s/1.24.x
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
modules/k3s/go.mod (1)
8-8: LGTM! Dependencies correctly promoted to direct requires.Moving
containerd/platformsandopencontainers/image-specto direct dependencies aligns with the new explicit imports inmodules/k3s/k3s.go.Also applies to: 11-11
modules/k3s/k3s.go (4)
10-10: LGTM! Imports support the new platform-aware API.The new imports for
containerd/platformsandopencontainers/image-specare necessary for the platform-specific image loading functionality.Also applies to: 14-14
196-199: LGTM! Platform-specific save options correctly applied.The conditional inclusion of platform-specific save options when a platform is provided is implemented correctly.
221-228: LGTM! Command construction with optional platform flag is correct.The incremental command building with conditional
--platformflag is clear and correctly usesplatforms.Format()to format the platform specification.
210-210: Verify SaveImagesWithOpts context usage
modules/k3s/k3s.go:210 usescontext.Background()instead of the caller’sctx, so cancellation won’t propagate. Replace withctxor document why a background context is required.
LaurentGoderre
force-pushed
the
fix-k3s-load-images
branch
from
c4b42ac to
49fa463
Compare
LaurentGoderre
force-pushed
the
fix-k3s-load-images
branch
2 times, most recently
from
cd99afd to
a2d5b8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docker.go (1)
1849-1851: Fix the option error messageThe failure text still says “save image option”; please rename it to “pull image option” so the diagnostic matches this code path.
- if err := opt(&pullOpts); err != nil { - return fmt.Errorf("applying save image option: %w", err) + if err := opt(&pullOpts); err != nil { + return fmt.Errorf("applying pull image option: %w", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docker.go(1 hunks)image.go(2 hunks)modules/k3s/go.mod(1 hunks)modules/k3s/k3s.go(3 hunks)modules/k3s/k3s_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/k3s/go.mod
🧰 Additional context used
🧬 Code graph analysis (3)
docker.go (1)
image.go (1)
PullImageOption(26-26)
modules/k3s/k3s_test.go (3)
modules/k3s/k3s.go (1)
Run(58-99)docker.go (1)
PullDockerImageWithPlatform(1857-1863)testing.go (1)
CleanupContainer(91-97)
modules/k3s/k3s.go (3)
provider.go (1)
ProviderDocker(18-18)image.go (1)
SaveImageOption(20-20)docker.go (1)
SaveDockerImageWithPlatforms(1831-1837)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint (modules/dolt) / lint: modules/dolt
- GitHub Check: lint (modules/redpanda) / lint: modules/redpanda
- GitHub Check: Analyze (go)
LaurentGoderre
force-pushed
the
fix-k3s-load-images
branch
2 times, most recently
from
15e7e07 to
f204382
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docker.go(1 hunks)image.go(2 hunks)modules/k3s/go.mod(1 hunks)modules/k3s/k3s.go(3 hunks)modules/k3s/k3s_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/k3s/go.mod
🧰 Additional context used
🧬 Code graph analysis (3)
modules/k3s/k3s.go (3)
provider.go (1)
ProviderDocker(18-18)image.go (1)
SaveImageOption(20-20)docker.go (1)
SaveDockerImageWithPlatforms(1831-1837)
modules/k3s/k3s_test.go (4)
modules/k3s/k3s.go (1)
Run(58-99)docker.go (1)
PullDockerImageWithPlatform(1857-1863)testing.go (1)
CleanupContainer(91-97)provider.go (1)
ProviderDocker(18-18)
docker.go (1)
image.go (1)
PullImageOption(26-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
image.go (2)
22-26: LGTM: Clean functional options pattern.The
pullImageOptionsstruct andPullImageOptionfunction type follow the same pattern as the existingSaveImageOption, maintaining consistency across the codebase.
34-34: Breaking change discussion ongoing.Adding
PullImageWithOptsto the exportedImageProviderinterface is a breaking change for downstream implementations. Based on the past review comments, this is being discussed with maintainers. The pattern is consistent with the existingSaveImagesWithOptsmethod already in the interface, but external implementations will need to add this method.If you haven't already, consider whether an optional interface pattern (as suggested in the past comments) would better preserve backward compatibility while still enabling this functionality for implementations that need it.
docker.go (1)
1857-1863: LGTM: Correct platform formatting.The factory function correctly uses
platforms.Format(platform)to convert the platform struct to the string format expected by Docker's pull API.modules/k3s/k3s_test.go (3)
43-49: Clever architecture-specific image selection.Constructing a single-architecture image tag (e.g., "amd64/nginx", "arm64v8/nginx") is a robust approach for testing scenarios where multi-arch images could mask platform-specific issues. This ensures the test exercises the actual platform-specific loading logic.
60-69: Good coverage for wrong-architecture scenario.This test verifies that attempting to load an image with a mismatched architecture fails gracefully. The regex
"content digest .* not found"matches the expected containerd error when the requested platform digest isn't present.
115-207: Comprehensive platform-aware loading tests.The new
Test_LoadImagesWithPlatformfunction provides thorough coverage of the new API:
- Non-existing images (error handling)
- Wrong architecture (platform mismatch validation with correct error message assertion)
- Successful in-cluster loading (end-to-end verification)
The past review comment about the assertion at lines 155-160 has been correctly addressed.
modules/k3s/k3s.go (3)
184-187: Good backward compatibility approach.Delegating
LoadImagestoLoadImagesWithPlatformwith a nil platform maintains the existing behavior while enabling the new functionality. This allows existing code to continue working unchanged.
189-226: Improved error handling in deprecated path.Lines 220-223 add error output capture when the
ctr importcommand fails, which significantly improves debuggability. Even though this method is deprecated, the improvement benefits users still relying on it during the transition period.
228-278: Well-structured platform-aware loading.The implementation correctly:
- Conditionally adds
SaveDockerImageWithPlatformsonly when a platform is specified (lines 236-238)- Builds the
ctr importcommand with an optional--platformflag (lines 260-266)- Uses
platforms.Format(*platform)to convert the platform struct to the string format expected by containerd (line 263)- Captures and returns command output on error for better diagnostics (lines 272-275)
The nil-check pattern allows callers to pass
nilfor default platform behavior while enabling explicit platform targeting when needed.
LaurentGoderre
force-pushed
the
fix-k3s-load-images
branch
from
f204382 to
67f7ac8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
modules/k3s/k3s_test.go (1)
117-143: Consider extracting common test setup to reduce duplication.The setup code for
Test_LoadImagesWithPlatform(lines 119-142) is nearly identical toTest_LoadImages(lines 24-42). Both create a k3s container, get kubeconfig, build a Kubernetes client, and get the Docker provider.Consider extracting this into a helper function:
func setupK3sTest(t *testing.T, ctx context.Context) (*k3s.K3sContainer, *kubernetes.Clientset, testcontainers.ContainerProvider) { t.Helper() k3sContainer, err := k3s.Run(ctx, "rancher/k3s:v1.27.1-k3s1") testcontainers.CleanupContainer(t, k3sContainer) require.NoError(t, err) kubeConfigYaml, err := k3sContainer.GetKubeConfig(ctx) require.NoError(t, err) restcfg, err := clientcmd.RESTConfigFromKubeConfig(kubeConfigYaml) require.NoError(t, err) k8s, err := kubernetes.NewForConfig(restcfg) require.NoError(t, err) provider, err := testcontainers.ProviderDocker.GetProvider() require.NoError(t, err) return k3sContainer, k8s, provider }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docker.go(1 hunks)image.go(2 hunks)modules/k3s/go.mod(1 hunks)modules/k3s/k3s.go(3 hunks)modules/k3s/k3s_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- modules/k3s/k3s.go
- docker.go
- image.go
🧰 Additional context used
🧬 Code graph analysis (1)
modules/k3s/k3s_test.go (3)
docker.go (2)
DockerProvider(977-984)PullDockerImageWithPlatform(1857-1863)testing.go (1)
CleanupContainer(91-97)provider.go (1)
ProviderDocker(18-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: lint (modules/grafana-lgtm) / lint: modules/grafana-lgtm
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
modules/k3s/k3s_test.go (4)
45-51: The architecture-specific image naming may be brittle.The code constructs platform-specific image names like
amd64/nginxusing the local platform's architecture. While this works for official Docker Hub images, this naming convention may not be universally supported and could fail for:
- Non-official images that don't follow this repo structure
- Platforms where Docker Hub doesn't provide architecture-specific repos
- Custom registries with different naming conventions
Consider documenting this limitation or adding a fallback mechanism if the test needs to work across a wider range of images and registries.
62-71: LGTM! Good coverage for cross-platform error case.The test correctly validates that loading an image pulled for a different platform (s390x) fails with an appropriate error message.
73-114: LGTM! Thorough validation of in-cluster loading.The test properly validates the complete flow: loading the platform-specific image into k3s, creating a pod with
PullNeverpolicy, and verifying the pod reaches running state.
167-210: LGTM! Comprehensive platform-aware loading test.The test thoroughly validates the new
LoadImagesWithPlatformAPI by loading an image with explicit platform specification and verifying it runs successfully in the cluster.modules/k3s/go.mod (1)
8-11: Dependency versions are up-to-date. containerd/platforms v0.2.1 is the latest stable release (v1.0.0-rc.1 is a pre-release) and opencontainers/image-spec v1.1.1 matches the latest.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
modules/k3s/k3s_test.go (3)
43-45: Nice: safe type assertion prevents panicsCapturing and asserting the ok result avoids nil deref when casting the provider.
118-141: Good: new platform-aware test scaffolding and safe castThe platform-aware setup and provider cast look correct.
152-167: LGTM: wrong-architecture assertion fixed as discussed previouslyFormatting with platforms.Format and asserting err.Error() contains the expected substring is correct.
🧹 Nitpick comments (4)
modules/k3s/k3s_test.go (4)
63-72: Handle Parse errors and relax brittle regex
- Check the error from platforms.Parse; ignoring it can mask typos and cause confusing failures later.
- The containerd error message can vary by version; consider a looser match to reduce flakiness.
Apply this diff:
- p, _ := platforms.Parse("linux/s390x") + p, perr := platforms.Parse("linux/s390x") + require.NoError(t, perr) @@ - require.Regexp(t, "content digest .* not found", err) + // tolerate minor wording differences across containerd versions + require.Regexp(t, `(content digest .* not found)|(no match for.*manifest)|(not found: .*digest)`, err.Error())
74-115: Good in-cluster verification; consider pinning tag for test stabilityThe flow is solid. To avoid “latest” drift over time, pin a known nginx tag (or digest) for deterministic results.
142-145: Optional: be explicit about platform on pull for symmetryUsing provider.PullImage is fine, but you could use PullImageWithOpts with platforms.DefaultSpec() for clarity and parity with the rest of the test.
146-150: Check Parse errorAvoid discarding the parse error for linux/amd64.
Apply this diff:
- p, _ := platforms.Parse("linux/amd64") + p, perr := platforms.Parse("linux/amd64") + require.NoError(t, perr)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/k3s/k3s_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/k3s/k3s_test.go (2)
docker.go (2)
DockerProvider(977-984)PullDockerImageWithPlatform(1857-1863)testing.go (1)
CleanupContainer(91-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: lint (modules/scylladb) / lint: modules/scylladb
- GitHub Check: lint (modules/nats) / lint: modules/nats
- GitHub Check: lint (modules/inbucket) / lint: modules/inbucket
- GitHub Check: lint (modules/influxdb) / lint: modules/influxdb
- GitHub Check: lint (modules/weaviate) / lint: modules/weaviate
- GitHub Check: lint (modules/gcloud) / lint: modules/gcloud
- GitHub Check: lint (modules/milvus) / lint: modules/milvus
- GitHub Check: lint (modules/redpanda) / lint: modules/redpanda
- GitHub Check: lint (modules/postgres) / lint: modules/postgres
- GitHub Check: lint (modules/mariadb) / lint: modules/mariadb
- GitHub Check: lint (modules/clickhouse) / lint: modules/clickhouse
- GitHub Check: lint (modules/k3s) / lint: modules/k3s
- GitHub Check: lint (modules/valkey) / lint: modules/valkey
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
modules/k3s/k3s_test.go (3)
9-9: Import looks goodBrings in containerd/platforms needed for platform formatting/parsing.
55-55: Pull is fineExplicit pre-pull of the single-arch ref keeps the subsequent import path deterministic.
170-173: LGTM: explicit linux platform for in-cluster loadForcing OS=linux on the default spec is appropriate here.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
modules/k3s/k3s_test.go (1)
46-52: Duplicate: Fix arch-specific Docker Hub repo prefixes.This issue was already flagged in a previous review. The current code produces invalid Docker Hub repository names (e.g., "arm64/nginx" instead of "arm64v8/nginx"), causing 404 errors on non-amd64 platforms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/k3s/k3s_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/k3s/k3s_test.go (2)
docker.go (2)
DockerProvider(977-984)PullDockerImageWithPlatform(1857-1863)testing.go (1)
CleanupContainer(91-97)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint (modules/cockroachdb) / lint: modules/cockroachdb
- GitHub Check: lint (modules/memcached) / lint: modules/memcached
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
modules/k3s/k3s_test.go (5)
43-44: LGTM! Type assertion now properly checked.The type assertion correctly captures the boolean result and validates it before use, preventing potential nil pointer panics.
63-73: LGTM! Wrong architecture test is correctly structured.The test appropriately validates that the legacy
LoadImagesAPI fails when attempting to load an image that only has a specific platform digest available locally.
75-117: LGTM! In-cluster image loading test is well-structured.The test correctly validates the complete flow: loading an image into the cluster, creating a Pod with
PullNever, and verifying it reaches the Running state. Note that this test depends on the arch-specific image name from lines 46-52 being corrected.
140-141: LGTM! Type assertion properly validated.The type assertion is now handled safely with proper boolean checking.
171-214: LGTM! Platform-aware image loading test is correctly implemented.The test properly uses
LoadImagesWithPlatformwith the default platform spec, ensuring the correct architecture-specific image is loaded and can be used in the cluster withPullNever.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
modules/k3s/k3s_test.go (3)
140-142: Good: safe provider cast againConsistent guard avoids nil deref.
46-53: Fix arch-specific Docker Hub repo prefixes (arm/arm64 currently 404) or avoid themConcatenating Architecture+Variant yields invalid repos like arm64/nginx and armv7/nginx. Map to Docker Library prefixes (arm64v8, arm32v7/v6, etc.) or skip unsupported variants.
Apply this mapping:
- // This function only works for single architecture images - // Forces the test to use a single-arch version of the image - arch := platforms.DefaultSpec().Architecture - if platforms.DefaultSpec().Variant != "" { - arch += platforms.DefaultSpec().Variant - } - nginxImg := arch + "/nginx" + // LoadImages (legacy) requires single‑arch images; use existing arch-specific repos. + spec := platforms.DefaultSpec() + var repoPrefix string + switch spec.Architecture { + case "amd64": + repoPrefix = "amd64" + case "arm64": + repoPrefix = "arm64v8" + case "arm": + switch spec.Variant { + case "v7": + repoPrefix = "arm32v7" + case "v6": + repoPrefix = "arm32v6" + default: + t.Skipf("no single-arch nginx repo for %s", platforms.Format(spec)) + } + case "ppc64le": + repoPrefix = "ppc64le" + case "s390x": + repoPrefix = "s390x" + default: + t.Skipf("no single-arch nginx repo for %s", platforms.Format(spec)) + } + nginxImg := fmt.Sprintf("%s/nginx", repoPrefix)Optional: avoid per‑arch repos entirely by pulling "nginx" with an explicit platform via dockerProvider.PullImageWithOpts, then using LoadImages on that tag.
155-169: Handle platforms.Parse errors and avoid err shadowingBoth Parse calls ignore errors, and Line 161 re‑declares err. Fail fast on parse errors and reuse err.
- pullPlatform, _ := platforms.Parse("linux/s390x") + pullPlatform, err := platforms.Parse("linux/s390x") + require.NoError(t, err) img := "nginx:mainline" err = dockerProvider.PullImageWithOpts(ctx, img, testcontainers.PullDockerImageWithPlatform(pullPlatform)) require.NoError(t, err) - loadPlatform, _ := platforms.Parse("linux/amd64") - err := k3sContainer.LoadImagesWithPlatform(ctx, []string{img}, &loadPlatform) + loadPlatform, err := platforms.Parse("linux/amd64") + require.NoError(t, err) + err = k3sContainer.LoadImagesWithPlatform(ctx, []string{img}, &loadPlatform) require.Error(t, err) expected := fmt.Sprintf( "image with reference %s was found but does not provide the specified platform (%s)", img, platforms.Format(loadPlatform), ) require.Contains(t, err.Error(), expected)
🧹 Nitpick comments (1)
modules/k3s/k3s_test.go (1)
63-74: Make the error assertion less brittle across containerd/docker versionsThe regex "content digest .* not found" can vary by version. Prefer a stable substring check.
- require.Regexp(t, "content digest .* not found", err) + require.ErrorContains(t, err, "not found")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
modules/k3s/k3s_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
modules/k3s/k3s_test.go (4)
docker.go (2)
DockerProvider(977-984)PullDockerImageWithPlatform(1857-1863)modules/k3s/k3s.go (1)
Run(58-99)testing.go (1)
CleanupContainer(91-97)provider.go (1)
ProviderDocker(18-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint (modules/k3s) / lint: modules/k3s
- GitHub Check: lint (modules/vearch) / lint: modules/vearch
- GitHub Check: lint (modules/chroma) / lint: modules/chroma
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
modules/k3s/k3s_test.go (7)
9-9: LGTM: platform utilities importImporting github.com/containerd/platforms is appropriate for OS/arch handling.
43-45: Good: safe type assertion with guardUsing the two-value assertion and require.True prevents panics.
55-56: LGTM: pre-pulling ensures deterministic local availabilityPulling the test image up front reduces flakiness.
75-117: LGTM: end-to-end in-cluster validationSolid readiness loop; PullNever ensures we actually loaded the image.
119-127: LGTM: platform-aware test harnessScoped deadline, cleanup, and client wiring look good.
147-153: LGTM: negative path for non-existing image with explicit platformCovers the intended error case for LoadImagesWithPlatform.
172-175: LGTM: explicit OS for platform specForcing linux OS matches the k3s cluster runtime.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docker.go (1)
1857-1864: Improve documentation comment for clarity.The godoc comment on line 1857 is grammatically incomplete. It should clarify that this function returns an option.
Apply this diff:
-// PullDockerImageWithPlatform option that configures the pull image options to use the specified platform +// PullDockerImageWithPlatform returns an option that configures the pull image options to use the specified platform func PullDockerImageWithPlatform(platform specs.Platform) PullImageOption {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docker.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
docker.go (1)
image.go (1)
PullImageOption(26-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: lint (modules/milvus) / lint: modules/milvus
- GitHub Check: lint (modules/surrealdb) / lint: modules/surrealdb
- GitHub Check: lint (modules/yugabytedb) / lint: modules/yugabytedb
- GitHub Check: lint (modules/azure) / lint: modules/azure
- GitHub Check: lint (modules/nebulagraph) / lint: modules/nebulagraph
- GitHub Check: lint (modules/toxiproxy) / lint: modules/toxiproxy
- GitHub Check: lint (modules/consul) / lint: modules/consul
- GitHub Check: lint (modules/memcached) / lint: modules/memcached
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
docker.go (1)
1840-1842: LGTM! Clean delegation pattern.The refactor to delegate to
PullImageWithOptsmaintains backward compatibility while avoiding code duplication.
| // PullImage pulls image from registry, passing options to the provider | ||
| func (p *DockerProvider) PullImageWithOpts(ctx context.Context, img string, opts ...PullImageOption) error { | ||
| pullOpts := pullImageOptions{} | ||
|
|
||
| for _, opt := range opts { | ||
| if err := opt(&pullOpts); err != nil { | ||
| return fmt.Errorf("applying pull image option: %w", err) | ||
| } | ||
| } | ||
|
|
||
| return p.attemptToPullImage(ctx, img, pullOpts.dockerPullOpts) | ||
| } |
There was a problem hiding this comment.
Fix documentation comment.
Line 1844 comment says "PullImage" but the method name is "PullImageWithOpts".
Apply this diff:
-// PullImage pulls image from registry, passing options to the provider
+// PullImageWithOpts pulls image from registry, passing options to the provider
func (p *DockerProvider) PullImageWithOpts(ctx context.Context, img string, opts ...PullImageOption) error {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // PullImage pulls image from registry, passing options to the provider | |
| func (p *DockerProvider) PullImageWithOpts(ctx context.Context, img string, opts ...PullImageOption) error { | |
| pullOpts := pullImageOptions{} | |
| for _, opt := range opts { | |
| if err := opt(&pullOpts); err != nil { | |
| return fmt.Errorf("applying pull image option: %w", err) | |
| } | |
| } | |
| return p.attemptToPullImage(ctx, img, pullOpts.dockerPullOpts) | |
| } | |
| // PullImageWithOpts pulls image from registry, passing options to the provider | |
| func (p *DockerProvider) PullImageWithOpts(ctx context.Context, img string, opts ...PullImageOption) error { | |
| pullOpts := pullImageOptions{} | |
| for _, opt := range opts { | |
| if err := opt(&pullOpts); err != nil { | |
| return fmt.Errorf("applying pull image option: %w", err) | |
| } | |
| } | |
| return p.attemptToPullImage(ctx, img, pullOpts.dockerPullOpts) | |
| } |
🤖 Prompt for AI Agents
In docker.go around lines 1844 to 1855, the top-of-function comment incorrectly
reads "PullImage" while the function is named PullImageWithOpts; update the
documentation comment to match the function name (e.g., "PullImageWithOpts pulls
an image from the registry, passing options to the provider") and ensure the
comment uses the Go doc convention (starts with the exact function name).
deprecate LoadImagesWithOpts with a new LoadImagesWithPlatform
LaurentGoderre
force-pushed
the
fix-k3s-load-images
branch
from
3c2b841 to
e2b9db1
Compare
2 participants
What does this PR do?
This PR adds more robust error handling around image loading and replaces the LoadImagesWithOpt that uses
--all-platformto aLoadImagesWithPlatformfunction that allows loading the platform correctly.This is required because
--all-platformfails when some digests for some platform are missing.Why is it important?
This is important because loading an image without specifying a platform is now broken.